Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup PR #69

Merged
merged 14 commits into from
Jan 21, 2025
Merged

Cleanup PR #69

merged 14 commits into from
Jan 21, 2025

Conversation

JonhasSC
Copy link
Collaborator

@JonhasSC JonhasSC commented Jan 10, 2025

This PR is focused on cleaning up a bit of #68 and #65. Estimate resources handles gate accuracy differently when it comes to performing cpt transformations or through quatran manipulation, which wasn't totally clear. This resulted in negative resources when using the default argument, 10. The intended usage was to express it as 1e-10, but pyLIQTR does not handle it that way.

On top of those bug fixes, does some QOL changes for the scripts.

value_per_t_gate is handled better as well

Copy link
Collaborator

@zain2864 zain2864 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few comments, some functionality was already tested when doing RE's for Dynamics and GSEE, other than that, it looks good to me.


gen_json(logical_re, outfile, metadata )
return cpt_trotter
logical_re = estimate_cpt_resources(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, I did some testing with using Trotter for Dynamics, if use_analytical = False, but given both value and reps, the value_per_t_gate is recorded as null. Is this the correct usage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bug, great job finding it. Fixed with the latest update

@@ -28,6 +29,7 @@ class EstimateMetaData:
is_extrapolated: bool=field(default=False, kw_only=True)
gate_synth_accuracy: int | float = field(default=10,kw_only=True)
value_per_circuit: float | None=field(default=None, kw_only=True)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we keeping this as value_per_circuit, or just changing to value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to value

… trotterized subprocess gsee to AFTER we create circuit
…up some code to make sure we getting right value per t gate
Copy link
Collaborator

@zain2864 zain2864 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some simple comments, and if everything else has been addressed from before, then this LGTM.

scripts/DickeModel.py Show resolved Hide resolved

def get_args():
parser = ArgumentParser()
parser.add_argument('-l', '--lattice_size', type=int, help='Lattice size')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we follow any convention here where the '-L' type of arguments should be capitalized? below in evolution time it is lowercase too

@JonhasSC JonhasSC merged commit 8eacdd7 into dev Jan 21, 2025
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants